[fix](iceberg) Harden external-write lifecycle, OCC, memory, and file ownership - #66348
Open
Gabriel39 wants to merge 24 commits into
Open
[fix](iceberg) Harden external-write lifecycle, OCC, memory, and file ownership#66348Gabriel39 wants to merge 24 commits into
Gabriel39 wants to merge 24 commits into
Conversation
Gabriel39
requested review from
924060929,
englefly,
gavinchou,
liaoxin01,
luwei16,
morningman,
morrySnow and
starocean999
as code owners
August 1, 2026 08:16
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Requesting changes: the static review found nine actionable issues (six P1, three P2), including three paths that can delete or reference missing Iceberg data, a concurrent use-after-free, an aggregate Thrift limit bypass, pathological spill reservation, and two incomplete object-storage cleanup paths.
Critical checkpoint conclusions:
- Goal and proof: The PR partially hardens write/cleanup/OCC behavior, and the overwrite and external-insert lifecycle changes are credible, but the nine inline findings prevent the stated lifecycle and cleanup goals from being met. Added tests prove only narrow single-state, AWS-abort, permutation, and executor cases.
- Scope and clarity: The edits are generally focused, but the new destructive action reimplements Iceberg safety policy incompletely, and the provider-neutral abort surface is implemented with AWS-specific state.
- Concurrency: The async writer publishes active-writer snapshots while a separately scheduled revocation task reads them. Per-writer
_sorter_mutexlocking is consistent and no deadlock was found, but it cannot protect the snapshot container because the range-for drops its temporary owner before iteration (MAIN-BE-003). - Lifecycle: Sort-writer close/cancel/deferred cleanup and external commit/listener ordering were traced end to end and otherwise preserve error ownership. S3/Azure abort lifecycle is incomplete: a rejected abort is never retried, and Azure reports successful abandonment without provider cleanup (MAIN-IO-001/002).
- Configuration: No new configuration was added. Existing
thrift_max_message_sizeand S3 limiter settings are read dynamically, but their consumers do not enforce the correct aggregate/cleanup contracts. - Compatibility: No storage-format or wire-field change was introduced. Default virtual methods preserve source-level parallel implementations, but that default is exactly why Azure silently lacks the new cleanup behavior.
- Parallel paths: Static/dynamic/branch/empty overwrites, data/delete writers, sync/async/destructor close, S3/Azure providers, and all insert executors were checked. The accepted findings identify the paths where parity is missing.
- Conditional checks: Existing overwrite conflict conditions are sound. The orphan action is missing the required
gc.enabledfence, safe retention interval, canonical URI identity handling, and version-hint reachability. - Test coverage: Coverage is not comprehensive. Missing cases include multi-task aggregate reports, concurrent snapshot publication/traversal, many-partition reserve accounting, hard-limit abort rejection, Azure staged-block abort, and execution-level orphan deletion tests for GC, retention, URI aliases, and the version hint.
- Test results: The new assertions are logically consistent within their narrow scopes; no result files changed. Builds and tests were intentionally not run because the authoritative review prompt forbids them, so this conclusion is static-only.
- Observability: Existing timers/logs cover writer/spill/commit paths, and abort failures are warned. No additional standalone observability defect was found, but logging does not substitute for retained cleanup ownership or retry.
- Transaction and persistence: No Doris EditLog state is added. Iceberg overwrite OCC anchors for main/branch/empty/static/dynamic cases are sound, and the durable external commit marker precedes refresh/listener suppression. The orphan action can nevertheless race a concurrent external commit (MAIN-FE-004).
- Data writes and atomicity: The aggregate-report failure can strand uncommitted objects; unsafe orphan deletion can corrupt shared, aliased, or concurrent writes; and multipart cleanup can leak provider state. These are blocking write-lifecycle defects.
- FE/BE propagation: No new FE-BE variable or Thrift field is introduced. Existing commit metadata is propagated on every producer path, but its budget is enforced per task rather than on the final aggregated RPC (MAIN-BE-001).
- Performance: Compact permutation dispatch is semantically sound, and bounded snapshot-copy cost is not independently material. Summing one full-batch reserve estimate per active partition can request roughly 128 blocks and repeatedly spill tiny partitions (MAIN-BE-002).
- Other issues: Error precedence, close idempotence, spill cleanup, overwrite validation, permission checks, location containment, and post-commit listener behavior produced no additional substantiated defects.
User focus: review_focus.txt contained no additional focus guidance, so the whole PR was reviewed without a narrower focus.
Review status: complete and converged in round 3. All final full-scope and risk-focused reviewers returned NO_NEW_VALUABLE_FINDINGS for this exact comment set. Static review only; no builds or tests were run.
Contributor
FE UT Coverage ReportIncrement line coverage |
Contributor
TPC-H: Total hot run time: 29107 ms |
Contributor
TPC-DS: Total hot run time: 169793 ms |
Contributor
ClickBench: Total hot run time: 23.92 s |
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
FE Regression Coverage ReportIncrement line coverage |
Gabriel39
added a commit
to Gabriel39/incubator-doris
that referenced
this pull request
Aug 1, 2026
### What problem does this PR solve? Issue Number: None Related PR: apache#66348 Problem Summary: Parallel Iceberg sink tasks could exceed the shared report limit, partition writers could over-reserve memory or outlive a temporary snapshot, failed multipart uploads could skip mandatory cleanup, and orphan-file deletion lacked Iceberg safety fences. Share the report budget, retain writer snapshots, bound reservations, make abort cleanup provider-aware, and enforce safe canonical orphan detection. ### Release note Improve Iceberg write failure cleanup and orphan-file deletion safety. ### Check List (For Author) - Test: Unit Test - BE ASAN focused tests with Azure support enabled - Full fe-connector-iceberg unit test suite - Behavior changed: Yes, unsafe orphan cutoffs and GC-disabled deletion are rejected, and failed multipart uploads are cleaned up reliably. - Does this need documentation: No
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Request changes: two distinct issues remain after the current fixes: Azure abort can delete a valid pre-existing Put Blob object, and orphan-file reachability repeatedly scans inherited manifests with quadratic cost. I did not duplicate the existing review threads.
Critical checkpoint conclusions:
- Goal and proof: The PR broadly hardens Iceberg write/cleanup lifecycle, OCC, reporting limits, and orphan cleanup, but the Azure data-preservation goal is not yet met. The added tests cover nominal cleanup and OCC paths, not the two cases below.
- Scope and clarity: The changes are cohesive for the stated lifecycle work. The orphan action's hand-built reachability scan is the one area that should reuse Iceberg's unique-manifest approach.
- Concurrency: Async writer publication is protected by retained shared snapshots and per-sorter locking; the shared report budget has a consistent lock order. Azure's committed-block test is not a valid ownership fence for an existing object.
- Lifecycle: FE before-execution cleanup and post-commit error boundaries are sound, as are the reviewed nested writer close paths. Azure abort still destroys state it does not own.
- Configuration: No new configuration behavior requires dynamic-reload handling.
- Compatibility: No FE-BE or storage-format incompatibility was found. The changed Azure block-ID format has a bounded retry impact and did not survive as a separate finding.
- Parallel paths: S3, Azure, single-shot, multipart, committer, overwrite, branch, static/dynamic overwrite, and delete/merge paths were traced; the surviving provider-specific issue is inline.
- Conditional checks: GC, retention, URI containment, identity ambiguity, and overwrite validation checks are fail-closed.
CommittedBlocks.empty()is the invalid special case. - Test coverage: Unit coverage is substantial but misses preservation of an existing Put Blob target and an append-heavy multi-snapshot reachability scan. No end-to-end result covers either.
- Test results: No tests or builds were run because the authoritative review task is static-only; no generated result files were changed.
- Observability: Existing status/log paths are adequate for the reviewed failures; no additional metric was required for a correctness conclusion.
- Transactions and persistence: Iceberg overwrite OCC, including empty-table and branch anchors, is sound. No EditLog or master-failover persistence change is involved.
- Data writes: Commit/report cleanup ownership is sound outside the Azure abort branch; that branch can lose previously committed data and blocks approval.
- FE-BE variables: No new cross-version transmitted variable or missing propagation path was found.
- Performance: Per-snapshot
planFiles()causes the P2 quadratic manifest-read issue below; no other distinct hot-path issue survived duplicate suppression. - Other issues: The sorter aggregate-reservation concern remains covered by the existing thread, and cleanup admission/terminal-state concerns remain covered by the existing Azure/rate-limit threads, so they are not repeated here.
User focus: no additional user-provided focus was present; the full PR was reviewed.
Contributor
FE UT Coverage ReportIncrement line coverage |
Contributor
TPC-H: Total hot run time: 29004 ms |
Contributor
TPC-DS: Total hot run time: 169560 ms |
Contributor
ClickBench: Total hot run time: 23.98 s |
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
FE Regression Coverage ReportIncrement line coverage |
### What problem does this PR solve? Issue Number: None Related PR: apache#66348 Problem Summary: Parallel Iceberg sink tasks could exceed the shared report limit, partition writers could over-reserve memory or outlive a temporary snapshot, failed multipart uploads could skip mandatory cleanup, and orphan-file deletion lacked Iceberg safety fences. Share the report budget, retain writer snapshots, bound reservations, make abort cleanup provider-aware, and enforce safe canonical orphan detection. ### Release note Improve Iceberg write failure cleanup and orphan-file deletion safety. ### Check List (For Author) - Test: Unit Test - BE ASAN focused tests with Azure support enabled - Full fe-connector-iceberg unit test suite - Behavior changed: Yes, unsafe orphan cutoffs and GC-disabled deletion are rejected, and failed multipart uploads are cleaned up reliably. - Does this need documentation: No
### What problem does this PR solve? Issue Number: None Related PR: apache#66348 Problem Summary: Azure abort could delete a pre-existing Put Blob object because an empty committed block list does not mean the key has no committed content. Iceberg orphan cleanup also reopened inherited data manifests once per retained snapshot, making long histories quadratic. Preserve Put Blob content while discarding only staged block-list replacements, and scan each unique retained manifest once. ### Release note Preserve existing Azure objects during failed Iceberg replacements and bound orphan cleanup manifest reads. ### Check List (For Author) - Test: Unit Test - Azure abort helper BE unit test - Iceberg orphan action FE unit tests - Behavior changed: Yes, Azure abort no longer deletes pre-existing Put Blob content and orphan cleanup deduplicates manifest reads. - Does this need documentation: No
Issue Number: None Related PR: apache#66348 Problem Summary: The latest review found that async memory admission ended before queued writes, Azure temporary-copy publication and its compatibility mirror were unsafe for same-key writers, BE did not know FE smaller Thrift limit, branch writes could lose the exact MVCC pin, and orphan cleanup assumed ownership for layouts it could not prove. Transfer reservations across the async queue, publish Azure block lists atomically on the target, negotiate the receiver limit, bind branch-aware pins after the target branch is known, and use ownership-aware bounded orphan scans. Harden Iceberg async write admission, Azure multipart publication, and orphan-file cleanup safety. - Test: Unit Test - BE targeted unit tests: 7 passed. - FE targeted unit tests: 37 passed; all 59 reactor modules built successfully. - Azure-enabled production and test translation units compiled successfully. - Behavior changed: Yes. Unsafe orphan scan roots now require an explicit guarded override, and conflicting Azure same-key multipart writers fail closed instead of mixing data. - Does this need documentation: No
### What problem does this PR solve? Issue Number: None Related PR: apache#66348 Problem Summary: A standalone Iceberg DELETE bypassed the table-writer capability gate, so a new BE could create delete files for an old coordinator that cannot acknowledge ownership transfer. Share the report-acknowledgement validation across table and delete writers and reject the direct delete path before it can create files. ### Release note Prevent standalone Iceberg deletes from creating files when the coordinator cannot acknowledge external-file ownership transfer. ### Check List (For Author) - Test: Unit Test - VIcebergDeleteSinkTest.* - VIcebergTableWriterTest.RejectsCoordinatorWithoutExternalFileReportAck - Behavior changed: Yes. Mixed-version standalone Iceberg DELETE now fails before file creation when the coordinator lacks report acknowledgement support. - Does this need documentation: No
Issue Number: None Related PR: apache#66348 Problem Summary: Periodic reports could prematurely send external commit vectors, while final-report rejection and Hive pre-commit failures could lose the last cleanup owner for deferred object-store uploads. Keep ownership-bearing vectors final-only, retain provider cleanup callbacks until the final report is accepted, and self-rollback Hive validation or classification failures. Also align the orphan-action imports with the connector SPI package. None - Test: Unit Test - HiveConnectorTransactionTest: 17 tests passed - Iceberg connector focused suites: 283 tests passed - Azure object-storage extension suite: 26 tests passed - FE core focused suites: 30 tests passed - Changed BE production and unit-test objects compiled successfully - clang-format v16 and connector import gate passed - Behavior changed: Yes, external-write ownership is transferred only by an accepted final report - Does this need documentation: No
### What problem does this PR solve? Issue Number: None Related PR: apache#66348 Problem Summary: The external-file report tests covered periodic metadata suppression and deferred provider cleanup separately, but did not exercise their ownership handoff as one lifecycle. Add a Hive writer regression that verifies a successful close retains the exact pending upload, periodic reports omit it, final reports include it, and a definite coordinator rejection aborts the retained provider upload. ### Release note None ### Check List (For Author) - Test: Unit Test - VHivePartitionWriterReportLifecycleTest.* - RuntimeStateIcebergCommitDataTest.* - S3FileWriterTest.failedReportCleanupAbortsDeferredProviderUploadAfterClose - 8 focused ASAN BE tests passed - clang-format v16 check passed - Behavior changed: No - Does this need documentation: No
### What problem does this PR solve? Issue Number: None Related PR: apache#66348 Problem Summary: runtime_state.h exposed a pointer to TReportExecStatusParams without declaring the generated Thrift class. Translation units that included RuntimeState without FrontendService types failed the full BE build. Forward-declare the pointer-only type so the header is self-contained without importing the full frontend service header. ### Release note None ### Check List (For Author) - Test: Manual test - Reproduced the failing ANN translation-unit compile before the fix. - All four affected ANN translation units passed ASAN syntax-only compilation after the fix. - RuntimeState and PipelineFragmentContext translation units passed ASAN syntax-only compilation. - clang-format v16 check passed for the affected header. - Full local BE build and focused BE UT were attempted but blocked before source compilation by an incomplete external Arrow thirdparty installation. - Behavior changed: No - Does this need documentation: No
### What problem does this PR solve? Issue Number: None Related PR: apache#66348 Problem Summary: Iceberg sorted writes could either sum a full batch estimate for every active partition or under-account cold partition dispatch and near-capacity sorter growth. The existing regressions also tested several ownership helpers in isolation instead of exercising the asynchronous writer, EOS merge, real partition selection, and Azure final-report paths. Derive safe row and byte upper bounds from the actual incoming block, preserve cumulative growth for every sorter that can be touched, reserve cold dispatch copies conservatively, and use saturating arithmetic for all reservation totals. Add production-path tests for reservation transfer, terminal cleanup, merge fan-in, many-partition selection, and Azure provider cleanup. ### Release note Harden Iceberg sorted-write memory admission for many-partition input and asynchronous writer lifecycle boundaries. ### Check List (For Author) - Test: Unit Test - 29 focused ASAN BE tests passed across spill admission, async writer, Iceberg partition/table writers, Hive report lifecycle, sorter, and reservation transfer suites. - All affected production and test objects compiled successfully in the ASAN build. - All 12 affected C/C++ files passed clang-format 16. - Focused clang-tidy findings in the changed sorter code were fixed; full translation-unit analysis remains blocked by pre-existing unconditional static assertions in be/src/util/jni-util.h. - Behavior changed: Yes. Memory admission now uses the actual input rows and bytes while retaining safe cumulative growth and cold-writer bounds. - Does this need documentation: No
### What problem does this PR solve? Issue Number: None Related PR: apache#66348 Problem Summary: Parallel regression suites can share an MTMV database. The getJobName helper queried mv_infos(), which materializes external metadata for every MV in that database, so an unrelated transient catalog failure could fail job lookup in another suite. Query MV job metadata directly and scope the lookup by both database and MV name. ### Release note None ### Check List (For Author) - Test: Unit Test - SuiteJobLookupTest verifies the isolated jobs metadata query. - Regression framework tests: 4 passed. - Framework Java and Groovy compilation passed. - Behavior changed: No. Regression job lookup no longer evaluates unrelated MV status. - Does this need documentation: No
### What problem does this PR solve? Issue Number: None Related PR: apache#66348 Problem Summary: The Iceberg merge sink tests used a default mock runtime state after Iceberg writers began requiring coordinator acknowledgement of external-file reports. The inner table writer therefore rejected open before twelve tests reached the behavior they intended to verify. Use an ACK-capable mock runtime state for successful merge writer scenarios while preserving the fail-closed production check. ### Release note None ### Check List (For Author) - Test: Unit Test - Reproduced 12 failures in VIcebergMergeSinkTest before the fixture fix. - 15 focused ASAN tests passed after the fix, including the no-ACK rejection test. - The affected test source compiled with the ASAN BE test flags. - The affected C++ file passed clang-format 16. - Behavior changed: No. This updates the test coordinator capability only. - Does this need documentation: No
Gabriel39
force-pushed
the
fix/iceberg-write-safety
branch
from
August 6, 2026 15:16
4c63bf4 to
13cbc27
Compare
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
|
Codex automated review failed and did not complete. Error: All Codex review accounts are usage-limited; earliest retry is 2026-08-08T03:32:00Z. Please trigger /review again after that time. |
Contributor
TPC-H: Total hot run time: 29480 ms |
Contributor
TPC-DS: Total hot run time: 166979 ms |
Contributor
ClickBench: Total hot run time: 23.99 s |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary:
Iceberg and Hive external writes had correctness and resource-safety gaps across statement lifecycle, optimistic concurrency, asynchronous memory ownership, multipart publication, final-report ownership transfer, and orphan cleanup.
This PR:
Release note
Harden Iceberg and Hive external-write lifecycle, OCC conflict detection, memory admission, multipart publication, final-report ownership transfer, and orphan-file safety.
Testing
HiveConnectorTransactionTest: 17 passed, including validation/classification self-rollback and malformed-record isolation.
Iceberg connector focused suites: 295 passed after rebasing onto the latest Iceberg V3 changes.
Azure filesystem extension suite: 26 passed.
FE report acknowledgement, task-state, sink-binding, and insert-executor focused suites: 30 passed.
Rebased BE ASAN focused suites: 13 passed, covering final-only report ownership, deferred S3 cleanup, standalone DELETE ACK gating, active writer snapshots, and the upstream missing-partition-source validation.
External-file report lifecycle verification: 8/8 ASAN tests passed, covering successful Hive close, periodic metadata deferral, exact final pending-upload identity, and provider abort on definite final-report rejection.
Sorted-write admission and ownership verification: 29/29 ASAN tests passed, covering multi-partition row/byte bounds, cold writer dispatch, reservation overflow, async write/EOS transfer, terminal cleanup, real spill fan-in, real 128-partition selection, and Azure final-report rejection.
All pre-existing branch commits remained patch-equivalent after rebasing onto master.
Header self-containment compile verification: all four affected ANN translation units, RuntimeState, and PipelineFragmentContext passed ASAN syntax-only compilation after adding the missing Thrift forward declaration.
All changed BE C/C++ files passed the repository clang-format v16 check; the latest 12-file update also passed clang-format v16 dry-run and
git diff --check.FE Checkstyle completed with 0 violations.
Connector forbidden-import architecture gate passed.
Focused clang-tidy found and fixed the changed sorter warning; full translation-unit analysis remains blocked by repository-baseline unconditional static assertions in
be/src/util/jni-util.h.Regression framework MTMV job lookup: the targeted test failed with the old cross-MV status query and passed with the isolated jobs metadata query; all 4 framework tests passed, with Java and Groovy compilation successful.
Iceberg merge sink BE UT: reproduced 12/14 failures with a mock coordinator missing report-ACK capability; after modeling an ACK-capable coordinator, all 14 merge sink tests passed, and the separate no-ACK fail-closed test also passed (15/15 focused ASAN tests).
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)